Skip to content

[HUDI-2417] Add support allowDuplicateInserts in HoodieJavaClient #3644

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
nsivabalan merged 3 commits into from
Jan 24, 2022
Merged

[HUDI-2417] Add support allowDuplicateInserts in HoodieJavaClient #3644

nsivabalan merged 3 commits into from
Jan 24, 2022

Conversation

@dongkelun
Copy link
Contributor

@dongkelun dongkelun commented Sep 11, 2021

Tips

What is the purpose of the pull request

Add support allowDuplicateInserts in HoodieJavaClient

Brief change log

(for example:)

  • Add support allowDuplicateInserts in HoodieJavaClient

Verify this pull request

This change added tests and can be verified as follows:

  • Added testHoodieConcatHandle in TestJavaCopyOnWriteActionExecutor

Committer checklist

  • Has a corresponding JIRA in PR title & commit

  • Commit message is descriptive of the change

  • CI is green

  • Necessary doc changes done or have another open PR

  • For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

@pratyakshsharma
Copy link
Contributor

pratyakshsharma commented Sep 12, 2021

LGTM!

@vinothchandar vinothchandar self-assigned this Sep 23, 2021
@vinothchandar
Copy link
Member

vinothchandar commented Sep 23, 2021

cc @yihua

#3696 is refactoring a bunch of this to ensure such cross client inconsistencies are minimized. Will take a look once we land that.

vinothchandar
vinothchandar reviewed Dec 21, 2021
View reviewed changes
Copy link
Member

@vinothchandar vinothchandar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments on the tests, but given the other PR is delayed. Might be okay to land this and reconcile. but still this is just duplicating code from the spark action executor

...ent/src/test/java/org/apache/hudi/table/action/commit/TestJavaCopyOnWriteActionExecutor.java Outdated
records2.add(new HoodieRecord(new HoodieKey(insertRow1.getRowKey(), insertRow1.getPartitionPath()), insertRow1));
records2.add(new HoodieRecord(new HoodieKey(insertRow2.getRowKey(), insertRow2.getPartitionPath()), insertRow2));

Thread.sleep(1000);
Copy link
Member

@vinothchandar vinothchandar Dec 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please avoid relying on sleep in tests. You can generate the commit times in the test, for greater control? Also could we move this test as a separate one for HoodieConcatHandle?

...ent/src/test/java/org/apache/hudi/table/action/commit/TestJavaCopyOnWriteActionExecutor.java Outdated
}

@Test
public void testHoodieConcatHandle() throws Exception {
Copy link
Member

@vinothchandar vinothchandar Dec 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondeing if this can be rewritten more nicely using the TestTable abstraction. cc @xushiyan .

@dongkelun dongkelun force-pushed the HUDI-2417 branch 5 times, most recently from 4feee2b to dc76423 Compare December 22, 2021 03:30
@dongkelun
Copy link
Contributor Author

dongkelun commented Dec 22, 2021

@vinothchandar Hello, I have added comments, removed sleep and rebase with latest master, and put the test cases in separate class: TestHoodieConcatHandle.Can you take a look?

nsivabalan
nsivabalan reviewed Jan 24, 2022
View reviewed changes
...di-java-client/src/test/java/org/apache/hudi/table/action/commit/TestHoodieConcatHandle.java Outdated
int index = 0;
for (GenericRecord record : fileRecords) {
assertEquals(records1.get(index).getRecordKey(), record.get("_row_key").toString());
if (index == 3) {
Copy link
Contributor

@nsivabalan nsivabalan Jan 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we also add assertion for index = 2. I assume "number" value should be 3.

Copy link
Contributor Author

@dongkelun dongkelun Jan 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll add it later

nsivabalan
nsivabalan requested changes Jan 24, 2022
View reviewed changes
Copy link
Contributor

@nsivabalan nsivabalan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes in general is good. just 1 nit. Can we try to see if we can leverage HoodieTestDataGenerator to write tests.
you can do something like this
dataGen.generateInserts : batch1
dataGen.generateUpdates : batch2
if not for allowDuplicates config set, total record count in snapshot view will be just batch1 records (assuming we updated all records from batch1)
if config is set to true, total record count in snapshot view will be batch1 records + batch2 records. and you can do some assertions to ensure both batch of records are present.

@dongkelun
Copy link
Contributor Author

dongkelun commented Jan 24, 2022

changes in general is good. just 1 nit. Can we try to see if we can leverage HoodieTestDataGenerator to write tests. you can do something like this dataGen.generateInserts : batch1 dataGen.generateUpdates : batch2 if not for allowDuplicates config set, total record count in snapshot view will be just batch1 records (assuming we updated all records from batch1) if config is set to true, total record count in snapshot view will be batch1 records + batch2 records. and you can do some assertions to ensure both batch of records are present.

OK, I'll try

@nsivabalan
Copy link
Contributor

nsivabalan commented Jan 24, 2022

since you have already put in time to write some tests, let already existing tests be there. may be in addition, give it a try.

@hudi-bot
Copy link
Collaborator

hudi-bot commented Jan 24, 2022

CI report:

Bot commands @hudi-bot supports the following commands:
  • @hudi-bot run azure re-run the last Azure build

@nsivabalan nsivabalan merged commit 1f7b6b2 into apache:master Jan 24, 2022
alexeykudinkin pushed a commit to onehouseinc/hudi that referenced this pull request Jan 25, 2022
@dongkelun
[HUDI-2417] Add support allowDuplicateInserts in HoodieJavaClient (ap…
ab545b3 
…ache#3644)
vingov pushed a commit to vingov/hudi that referenced this pull request Jan 26, 2022
@dongkelun
[HUDI-2417] Add support allowDuplicateInserts in HoodieJavaClient (ap…
ca968e3 
…ache#3644)
liusenhua pushed a commit to liusenhua/hudi that referenced this pull request Mar 1, 2022
@dongkelun @liusenhua
[HUDI-2417] Add support allowDuplicateInserts in HoodieJavaClient (ap…
afcee40 
…ache#3644)
vingov pushed a commit to vingov/hudi that referenced this pull request Apr 3, 2022
@dongkelun
[HUDI-2417] Add support allowDuplicateInserts in HoodieJavaClient (ap…
14fec4e 
…ache#3644)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vinothchandar vinothchandar vinothchandar left review comments

@nsivabalan nsivabalan nsivabalan approved these changes

Assignees

@nsivabalan nsivabalan

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dongkelun @pratyakshsharma @vinothchandar @nsivabalan @hudi-bot